-
Notifications
You must be signed in to change notification settings - Fork 749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move EEI into EVM #2604
Move EEI into EVM #2604
Conversation
…y() not taking options into account
…ccount-exists differentiation, return undefined in SM for not existing accounts, throw early for unintended non-account accesses
…nd taken in for new state root) due to new diff-based cache structure
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
c943f09
to
ad17af7
Compare
copy(): StateManagerInterface | ||
flush(): Promise<void> | ||
dumpStorage(address: Address): Promise<StorageDump> | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be really sparse with taking over interfaces to Common and not "throwing things all over here".
So these should really only be the main interfaces, like StateManagerInterface
, eventually BlockchainInterface
, and not much else.
So these should be very clear, very well-managed interfaces, where there is a lot of attention onto. Otherwise I fear that we will likely overload and produce too much complexity which won't make things easier to oversee respectively more stable.
I would particularly suggest that we do not expose the cache so prominently, I guess I added this (right?), but anyhow, it would likely be better if we just hack this a bit on the very few direct cache accesses we have internally than build up such heavy structures like this.
I would suggest the following:
- Remove cache from SM Interface, delete all related interfaces
- (CacheClearingOpts is outdated anyhow)
- Remove the
dumpStorage
andStorageDump
from the interface in general, this is only used in one very obscure client location, we can hack this in there - Throw the
StateAccess
andStateManagerInterface
interfaces together (never gotten a big fan of this) - Definitely remove this
AccountFields
partial - Eventually keep the Proof types, but really only if stable enough
Two other things:
- This could also very well be an own PR, totally independent from other stuff
- I think we should rather do a dedicated file
interfaces.ts
for this
* This is used the warn users that if they do not provide a blockchain, | ||
* the default blockchain will be used, which always returns the 0 hash if a block is | ||
*/ | ||
enableDefaultBlockchain?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this option.
I somewhat understand the intention to "warn" users. I would argue that this is somewhat intuitive though (if an EVM has no connection to whatever blockchain, no information from a/the blockchain can be read). So I think if we have this prominently in the docs that really should be enough.
At the same time the option causes a lot of bloat - like in the tests.
Hi Jochem, I also have the impression that there are several subtasks involved here which might be able to be separated, like:
Not sure if it might be worth to think a bit again about the order of tasks and eventually submit separately? Whatever is best and easier to realize. Just feels a bit several things are mixed here together. |
Another thing: I would really like to go through the Would be good if we think this a bit along here, do you think this work has any bigger implications on this? 🤔 Just had a look, you didn't touch VMState that much, right? We might also not need to finish this work before breaking, just make sure we have the structures in place (so do not expose VMState in any way or encourage usage or customization (not sure if we are doing this now?), the StateManager interface might also need some additional adjustments (so the So that we have all the breaking things in place. 🙂 |
Maybe we should just do some call on this and discuss this directly 😋, maybe on Thursday morning? |
Follow-up PR #2649 |
The purpose of this PR is to move the EEI into EVM and perform cleanups.
Note: this targets the
statemanager-cache-rewrite-new
, which currently points tomaster
, but this PR is super breaking and it should eventually land indev-v7
.